-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory issues #1113
Merged
Fix memory issues #1113
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bors try |
bors try |
tryBuild failed: |
simonbyrne
approved these changes
Feb 1, 2023
dennisYatunin
force-pushed
the
dy/memory
branch
from
February 1, 2023 22:36
c543dcd
to
5ee6904
Compare
bors r+ |
I think it would have been better if we had added new tests in this PR, and converted any passing tests to failing tests, rather than change existing tests, which makes understanding the changes more complicated. |
1 task
This was referenced Apr 10, 2023
Merged
This was referenced Apr 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reduces ClimaCore memory usage due to excessive code inlining, and it closes #1064.
Specifically, it
@inline
annotations toBase.copyto!
that were added in Inlinecopyto!
to fix Ref allocations #945 and Extend and fix Ref broadcasting with StencilCoefs #978.slurm_mem
increases from the CI pipeline, so that the default value of 8GB applies to every task.Field
s andDataLayout
s so that it can handle single-elementTuple
s. In theory, broadcasting with a single-elementTuple
should be equivalent to broadcasting with aRef
. However, since aRef
is mutable, it looks like the compiler needs to perform runtime allocations when using aRef
, unless we force it to inline the majority of the broadcasting machinery.Ref
intest/Fields/field_opt.jl
with a single-elementTuple
, so that the runtime allocation unit tests can pass.@test_broken
s intest/Operators/finitedifference/opt_examples.jl
with@test
s, since those tests are now passing, even though they don't appear to have anything to do withRef
s.Note: It is still necessary to go through the rest of our code (both in ClimaCore and elsewhere) and replace every
Ref
in aField
/DataLayout
broadcast with a single-elementTuple
. Now that the@inline
annotations have been removed, broadcasting with aRef
will trigger runtime allocations, which will pose a problem for our long runs.